Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Umount the device first during mount_usb_storage() (#1587) (BugFix) #1599

Merged
merged 4 commits into from
Dec 3, 2024
Merged

Umount the device first during mount_usb_storage() (#1587) (BugFix) #1599

merged 4 commits into from
Dec 3, 2024

Conversation

Jefferyyen
Copy link
Contributor

Description

To prevent the inserted storage from failing to mount twice, first ensure it is unmounted.

Because the OS will auto-mount the inserted storage, the existing subprocess.call(["mount", device_to_mount, FOLDER_TO_MOUNT]) command will cause an error if the format of inserted storage is NTFS.

Perform an umount on device_to_mount (e.g., for the media card in #1587, it would be umount /dev/mmcblk0p1) before attempting to mount it again to avoid this issue.

Resolved issues

Fix (#1587)

Documentation

Tests

  1. Run checkbox-cli run com.canonical.certification::mediacard/sdhc-storage-manual
  2. Insert mediacard as prompt:

--------- Testing insertion ---------


INSERT NOW


Timeout: 30 seconds
  1. After detectd the card, checkbox will do the read/write test.

With the above change, it shows:

--------- Testing insertion --------

INSERT NOW


Timeout: 30 seconds
INFO: usable partition: mmcblk0p1
INFO: Device: ultra high speed SDR104 SDHC
INFO: Address: 59b4
INFO: Mediacard insertion test passed.

------- Insertion test passed -------

--------- Testing read/write --------
DEBUG:generating a random file
DEBUG:try to mount usb storage for testing
DEBUG:mount /dev/mmcblk0p1 on /tmp/tmpp76jsgp1 successfully.
DEBUG:===================
DEBUG:writing test begins
DEBUG:===================
DEBUG:Apply command: ['dd', 'if=/tmp/tmphkvvfmg9', 'of=/tmp/tmpp76jsgp1/tmphkvvfmg90', 'bs=1M', 'oflag=sync']
DEBUG:['100+1 records in', '100+1 records out', '104858730 bytes (105 MB, 100 MiB) copied, 7.43403 s, 14.1 MB/s', '']
DEBUG:No I/O errors found in dmesg

Without the change, it shows:


--------- Testing insertion --------

INSERT NOW


Timeout: 30 seconds
INFO: usable partition: mmcblk0p1
INFO: Device: ultra high speed SDR104 SDHC
INFO: Address: 59b4
INFO: Mediacard insertion test passed.

------- Insertion test passed -------

--------- Testing read/write --------
DEBUG:generating a random file
DEBUG:try to mount usb storage for testing
Mount is denied because the NTFS volume is already exclusively opened.
The volume may be already mounted, or another software may use it which
could be identified for example by the help of the 'fuser' command.
ERROR:mount /dev/mmcblk0p1 on /tmp/tmph37fxtlq failed.
INFO:context manager exit: unmount USB storage
umount: /tmp/tmph37fxtlq: not mounted.
WARNING:umount /tmp/tmph37fxtlq failed.
INFO:Remove temporary folders and files.

@fernando79513 fernando79513 changed the title Umount the device first during mount_usb_storage() (Bugfix) (#1587) Umount the device first during mount_usb_storage() (#1587) (BugFix) Nov 14, 2024
@fernando79513 fernando79513 self-assigned this Nov 18, 2024
@fernando79513 fernando79513 added the waiting-for-changes The review has been completed but the PR is waiting for changes from the author label Nov 18, 2024
fernando79513
fernando79513 reviewed Nov 18, 2024
View reviewed changes
Copy link
Collaborator

@fernando79513 fernando79513 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. I've just left a small comment there.

checkbox-support/checkbox_support/scripts/usb_read_write.py Outdated Show resolved Hide resolved
Jefferyyen
Jefferyyen commented Nov 26, 2024
View reviewed changes
Copy link
Contributor Author

@Jefferyyen Jefferyyen left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for making the comments more readable.

Jefferyyen
Jefferyyen commented Nov 26, 2024
View reviewed changes
Copy link
Contributor Author

@Jefferyyen Jefferyyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update the comment.

Jefferyyen
Jefferyyen commented Nov 26, 2024
View reviewed changes
Copy link
Contributor Author

@Jefferyyen Jefferyyen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fernando79513 Thank you for the suggestion. Please check if it is okay.

@fernando79513 fernando79513 removed the waiting-for-changes The review has been completed but the PR is waiting for changes from the author label Nov 28, 2024
@fernando79513
Copy link
Collaborator

I've just seen that the first commit is not signed properly. Please sign the first commit and rebase this branch so all the comments are signed.

@Jefferyyen
Umount the device first during mount_usb_storage() (Bugfix) (canonica…
039d674 
…l#1587)

To prevent the inserted storage from failing to mount twice, first ensure it is unmounted.

Because the OS will auto-mount the inserted storage, the existing subprocess.call(["mount", device_to_mount, FOLDER_TO_MOUNT]) command will cause an error if the format of inserted storage is NTFS.

Perform an umount on device_to_mount (e.g., for the media card in canonical#1587, it would be umount /dev/mmcblk0p1) before attempting to mount it again to avoid this issue.
@Jefferyyen
Copy link
Contributor Author

Jefferyyen commented Dec 3, 2024 •
edited
Loading

Hi @fernando79513 ,
I have finished signing the 4 commits and push them to the branch. The commits are now signed and verified. Could you please help to review and check if it is fine for merging? Thank you so much once again.

@fernando79513 fernando79513 merged commit d0c208e into canonical:main Dec 3, 2024
35 checks passed
@scotthu27 scotthu27 mentioned this pull request Dec 20, 2024
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fernando79513 fernando79513 fernando79513 approved these changes

Assignees

@fernando79513 fernando79513

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Jefferyyen @fernando79513